fix: HTTP unknown methods #1781
Conversation
arielvalentin
requested review from
a team,
ahayworth,
dazuma,
ericmustin,
fbogsany,
kaylareopelle,
mwear,
plantfansam,
robbkidd,
robertlaurin,
simi and
xuan-cao-swi
as code owners
arielvalentin
force-pushed
the
http-methods-fix
branch
from
065f55f to
f59066e
Compare
| def self.span_attrs_for(method, semconv: :stable) | ||
| normalized = METHOD_CACHE[method] | ||
| if normalized | ||
| span_name = semconv == :old ? OLD_SPAN_NAMES[normalized] : normalized |
There was a problem hiding this comment.
@arielvalentin - I'm concerned about changing the span names and attributes for the "old" instrumentation. It seems to break the contract for the OTEL_SEMCONV_STABILITY_OPT_IN var:
The default behavior (in the absence of one of these values) is to continue emitting whatever version of the old experimental HTTP and networking conventions the instrumentation was emitting previously.
(source)
What do you think about reverting the changes for the old modules and leaving the changes for dup and new?
There was a problem hiding this comment.
Ok that makes sense. I can leave the span name alone in follow up PRs.
Please consider that the existing old implementation does not follow the correct conventions for span names and should not have a prefix of HTTP:
HTTP spans MUST follow the overall guidelines for span names. HTTP server span names SHOULD be {http.method} {http.route} if there is a (low-cardinality) http.route available. HTTP server span names SHOULD be {http.method} if there is no (low-cardinality) http.route available. HTTP client spans have no http.route attribute since client-side instrumentation is not generally aware of the "route", and therefore HTTP client spans SHOULD use {http.method}. Instrumentation MUST NOT default to using URI path as span name, but MAY provide hooks to allow custom logic to override the default span name.
There was a problem hiding this comment.
may provide hooks to allow custom logic to override the default span name is also something new to me 🤔
arielvalentin
force-pushed
the
http-methods-fix
branch
from
f59066e to
59be641
Compare
Ensures that all HTTP client gems support unknown methods as described in semantic conventions: https://opentelemetry.io/docs/specs/semconv/http/http-spans/#http-client-span See open-telemetry#1779
arielvalentin
force-pushed
the
http-methods-fix
branch
from
59be641 to
d33f60a
Compare
kaylareopelle
left a comment
kaylareopelle
left a comment
There was a problem hiding this comment.
I want to get @hannahramadan's opinion on this too, but I'm at the point where I feel comfortable pushing this fix to the old instrumentations. It's an unimplemented feature that we likely would have fixed if we noticed/had the time. The only request I have is to mark this as a breaking change so people can be alerted their span names are changing.
|
Hi @arielvalentin! I was initially a little iffy on the ethon span name change, but after chatting with @kaylareopelle, I agree that if we mark this as a breaking change, then fixing the span name ( |
3 participants
Ensures that all HTTP client gems support unknown methods as described in semantic conventions: https://opentelemetry.io/docs/specs/semconv/http/http-spans/#http-client-span
Some concessions:
_OTHERmethod cases by setting the span names toHTTPethongem for_OTHERto beHTTP N/AOTEL_INSTRUMENTATION_HTTP_KNOWN_METHODSenvironment variable to a future PRSee #1779